Repository navigation
Preserve iOS terminal shell while reconnecting - #6647
lawrencecchen wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces a generation-aware ChangesTerminal session lifecycle, replay, and reconnect shell preservation
Sequence Diagram(s)sequenceDiagram
participant Coordinator as GhosttySurfaceRepresentable.Coordinator
participant SurfaceView as GhosttySurfaceView
participant Session as TerminalSurfaceSessionState
participant Executor as GhosttySurfaceWorkExecutor
participant Composite as MobileShellComposite
participant RPC as mobile.terminal.replay
Coordinator->>SurfaceView: processOutputAndWait(data)
SurfaceView->>Session: requestRender / registerPendingOutputCompletion
SurfaceView->>Executor: async(surface:) ghostty_surface_process_output
Executor-->>SurfaceView: (MainActor) applied: Bool
SurfaceView->>Session: markOutputApplied / completeReplayAttempt
SurfaceView->>SurfaceView: completePendingOutput(applied:)
SurfaceView-->>Coordinator: Bool
alt not applied (dropped)
Coordinator->>Composite: terminalOutputDidDropForRetry(surfaceID, streamToken)
Composite->>Composite: resetQueue, removeQueuedEndSeq
end
SurfaceView->>SurfaceView: scheduleRecoveryReplayAttempt
SurfaceView->>Composite: ghosttySurfaceViewNeedsReplay(surfaceID)
Composite->>RPC: executeTerminalReplay (workspace_id, surface_id)
RPC-->>Composite: renderGrid / bytes
Composite->>Composite: check terminalOutputAcceptedEndSeq (stale?)
Composite->>SurfaceView: deliverTerminalRenderGrid / deliverTerminalBytes(endSeq:)
Composite-->>SurfaceView: performTerminalReplay → Bool
flowchart LR
hasCachedRemoteWorkspaceSnapshot --> shouldPreserveWorkspaceShellDuringReconnect
isRecoveringOrReconnecting --> shouldPreserveWorkspaceShellDuringReconnect
connectionState --> shouldPreserveWorkspaceShellDuringReconnect
shouldPreserveWorkspaceShellDuringReconnect --> rootContentDestination
shouldPreserveWorkspaceShellDuringReconnect --> bannerPresentation
shouldPreserveWorkspaceShellDuringReconnect --> WorkspaceListView
rootContentDestination --> CMUXMobileRootView
bannerPresentation --> MobileConnectionRecoveryBanner
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (9 errors, 1 warning)
✅ Passed checks (15 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR preserves the iOS workspace shell during transport reconnects by keeping
Confidence Score: 4/5Safe to merge with one follow-up: replace the The reconnect shell-preservation logic, per-generation executor refactor, and replay deduplication are well-structured and covered by new tests. One new file uses Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+ReplayRecovery.swift (asyncAfter sleep pattern in handleRecoveryReplayResult) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Transport disconnects] --> B{hasCachedRemoteWorkspaceSnapshot?}
B -- No --> C[Show RestoringStoredMac / Onboarding]
B -- Yes --> D{connectionRequiresReauth?}
D -- Yes --> C
D -- No --> E[Keep WorkspaceShellView mounted]
E --> F[Show Reconnecting banner over last Ghostty frame]
F --> G[Transport reconnects]
G --> H[replayMountedTerminalSinks]
H --> I[performTerminalReplay per surface]
I --> J{Surface sink present?}
J -- No --> K[return false]
J -- Yes --> L[deliverTerminalBytes / renderGrid]
L --> M[processOutputAndWait]
M --> N{applied?}
N -- Yes --> O[terminalOutputDidProcess - advance deliveredSeq]
N -- No --> P[terminalOutputDidDropForRetry - reset queue]
P --> Q{replay attempts remaining?}
Q -- Yes --> I
Q -- No --> R[failClosedSurfaceRecovery - show Retry banner]
K --> Q
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Transport disconnects] --> B{hasCachedRemoteWorkspaceSnapshot?}
B -- No --> C[Show RestoringStoredMac / Onboarding]
B -- Yes --> D{connectionRequiresReauth?}
D -- Yes --> C
D -- No --> E[Keep WorkspaceShellView mounted]
E --> F[Show Reconnecting banner over last Ghostty frame]
F --> G[Transport reconnects]
G --> H[replayMountedTerminalSinks]
H --> I[performTerminalReplay per surface]
I --> J{Surface sink present?}
J -- No --> K[return false]
J -- Yes --> L[deliverTerminalBytes / renderGrid]
L --> M[processOutputAndWait]
M --> N{applied?}
N -- Yes --> O[terminalOutputDidProcess - advance deliveredSeq]
N -- No --> P[terminalOutputDidDropForRetry - reset queue]
P --> Q{replay attempts remaining?}
Q -- Yes --> I
Q -- No --> R[failClosedSurfaceRecovery - show Retry banner]
K --> Q
Reviews (6): Last reviewed commit: "Test iOS terminal reconnect frame preser..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 209-214: Update the doc comment for the
`hasCachedRemoteWorkspaceSnapshot` property (currently at lines 209-214) to
clarify that the flag is only set to true when a connected Mac has returned
workspace snapshot(s) that contain terminals. The current comment states "at
least one workspace snapshot" but the actual implementation in the setter and
related code only sets this flag to true when workspaces contain terminals, so
the documentation should reflect this additional requirement to match the
implementation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 739bc01a-81aa-4205-b94e-7264eee6b2ce
📒 Files selected for processing (4)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositePreviewTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileConnectionRecoveryBanner.swift
| /// True after a connected Mac has returned at least one workspace snapshot. | ||
| /// | ||
| /// Ordinary network drops intentionally keep the last ``workspaces`` value so | ||
| /// the UI can keep rendering the local Ghostty mirror while the transport | ||
| /// reconnects. This flag distinguishes that real cached snapshot from the | ||
| /// preview/empty shell state used before a Mac has ever connected. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Clarify doc comment to mention terminals requirement.
The implementation at lines 216-217 and 5290-5291 only sets hasCachedRemoteWorkspaceSnapshot = true when workspaces contain terminals, but the doc comment at line 209 says "at least one workspace snapshot" without mentioning this requirement.
📝 Suggested doc clarification
- /// True after a connected Mac has returned at least one workspace snapshot.
+ /// True after a connected Mac has returned at least one workspace snapshot containing terminals.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// True after a connected Mac has returned at least one workspace snapshot. | |
| /// | |
| /// Ordinary network drops intentionally keep the last ``workspaces`` value so | |
| /// the UI can keep rendering the local Ghostty mirror while the transport | |
| /// reconnects. This flag distinguishes that real cached snapshot from the | |
| /// preview/empty shell state used before a Mac has ever connected. | |
| /// True after a connected Mac has returned at least one workspace snapshot containing terminals. | |
| /// | |
| /// Ordinary network drops intentionally keep the last ``workspaces`` value so | |
| /// the UI can keep rendering the local Ghostty mirror while the transport | |
| /// reconnects. This flag distinguishes that real cached snapshot from the | |
| /// preview/empty shell state used before a Mac has ever connected. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 209 - 214, Update the doc comment for the
`hasCachedRemoteWorkspaceSnapshot` property (currently at lines 209-214) to
clarify that the flag is only set to true when a connected Mac has returned
workspace snapshot(s) that contain terminals. The current comment states "at
least one workspace snapshot" but the actual implementation in the setter and
related code only sets this flag to true when workspaces contain terminals, so
the documentation should reflect this additional requirement to match the
implementation behavior.
| /// Test-only: drive the same recovery entry used by network foreground/path | ||
| /// changes without depending on NWPathMonitor timing. | ||
| func debugRecoverMobileConnectionForTesting() { | ||
| recoverMobileConnection(trigger: .networkChange) | ||
| } | ||
|
|
||
| /// Test-only: expose whether the SwiftUI-mounted Ghostty surface still has | ||
| /// an output consumer. This is the proxy for "the surface was not | ||
| /// dismantled", because `dismantleUIView` cancels the consuming task. | ||
| func debugHasTerminalOutputSinkForTesting(surfaceID: String) -> Bool { | ||
| hasTerminalOutputSink(surfaceID: surfaceID) | ||
| } |
There was a problem hiding this comment.
Test-only seams added to production
Sources/
debugRecoverMobileConnectionForTesting() and debugHasTerminalOutputSinkForTesting(surfaceID:) are named with the debug…ForTesting pattern explicitly forbidden by the no-test-debug-seam rule, and they live inside #if DEBUG in a production Sources/ path with no production callers. The #if DEBUG guard does not make a test-observability accessor acceptable in shipping source.
The canonical fix is to widen the underlying recoverMobileConnection(trigger:) and hasTerminalOutputSink(surfaceID:) helpers from private to internal and call them directly from the test target via @testable import, matching PR #6452's pattern.
Rule Used: Flag Swift files under a production Sources path (... (source)
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 4627-4629: Remove the debugRecoverMobileConnectionForTesting()
function entirely from the MobileShellComposite class, as it is a test-only
debug seam that should not exist in production code. Instead, ensure the
underlying recoverMobileConnection() method has internal visibility (or
appropriate access level) so that tests can access it directly through `@testable`
import. This eliminates the test wrapper pattern and keeps test accessibility
mechanisms within the Swift testing conventions.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+Testing.swift:
- Around line 1-8: The MobileShellComposite+Testing.swift file contains a
test-only seam with the debugHasTerminalOutputSinkForTesting method wrapped in
an `#if` DEBUG conditional, which violates the production-source seam policy for
Sources/ code. Remove the entire `#if` DEBUG block including the extension and the
debugHasTerminalOutputSinkForTesting wrapper method. Instead, rely on `@testable`
import in tests to directly access the internal hasTerminalOutputSink method,
eliminating the need for a test-only DEBUG seam in production source files.
In
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swift`:
- Around line 195-241: The test mounts a collector via the mount method but the
unmount cleanup call at the end can be skipped if an early assertion or
requirement fails (such as the `#require` calls), causing the mounted collector
task to persist and leak state into subsequent tests. Add a defer block
immediately after the collector.mount call to guarantee that collector.unmount
is always executed regardless of how the test exits, ensuring proper cleanup
even when early assertions fail.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileConnectionRecoveryBanner.swift`:
- Around line 60-67: The presentation computed property now depends on the
shouldPreserveWorkspaceShellDuringReconnect property in addition to the existing
dependencies, but the animation bindings are not watching this new property.
Find where the animation modifiers are applied (likely using .animation() or
similar SwiftUI animation bindings that currently watch isRecoveringConnection,
connectionRecoveryFailed, and connectionRequiresReauth) and add
store.shouldPreserveWorkspaceShellDuringReconnect to the list of observed values
so that banner transitions animate properly when the preserved-shell reconnect
state changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba2f0e7b-3c80-4462-8566-92381c0acce2
📒 Files selected for processing (12)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+PlaceholderHostName.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ReconnectPresentation.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+Testing.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView+DebugPreviews.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileConnectionRecoveryBanner.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileConnectionRecoveryBannerTests.swiftPackages/iOS/CmuxMobileWorkspace/Sources/CmuxMobileWorkspace/MobileRootAuthGate.swiftPackages/iOS/CmuxMobileWorkspace/Tests/CmuxMobileWorkspaceTests/MobileRootAuthGateTests.swift
| func debugRecoverMobileConnectionForTesting() { | ||
| recoverMobileConnection(trigger: .networkChange) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the test-only debug seam from production Sources code.
Line 4627 introduces debugRecoverMobileConnectionForTesting(), which is a test/debug accessor pattern in production code. Keep test access through @testable import + internal visibility on real members instead of adding ...ForTesting wrappers.
Suggested direction
- func debugRecoverMobileConnectionForTesting() {
- recoverMobileConnection(trigger: .networkChange)
- }As per path instructions, **/Sources/**/*.swift must flag test/debug seams in production files, including members named like debug… / …ForTesting.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func debugRecoverMobileConnectionForTesting() { | |
| recoverMobileConnection(trigger: .networkChange) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 4627 - 4629, Remove the debugRecoverMobileConnectionForTesting()
function entirely from the MobileShellComposite class, as it is a test-only
debug seam that should not exist in production code. Instead, ensure the
underlying recoverMobileConnection() method has internal visibility (or
appropriate access level) so that tests can access it directly through `@testable`
import. This eliminates the test wrapper pattern and keeps test accessibility
mechanisms within the Swift testing conventions.
Source: Path instructions
| #if DEBUG | ||
| extension MobileShellComposite { | ||
| /// Test-only: true while a mounted Ghostty surface still has an output consumer. | ||
| func debugHasTerminalOutputSinkForTesting(surfaceID: String) -> Bool { | ||
| hasTerminalOutputSink(surfaceID: surfaceID) | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the test-only DEBUG seam from production Sources/ code.
Lines 1-8 add a #if DEBUG accessor named debugHasTerminalOutputSinkForTesting, which violates the production-source seam policy. Prefer exposing only the minimal internal API and calling it from tests via @testable import instead of adding a ...ForTesting wrapper.
Suggested change
-#if DEBUG
-extension MobileShellComposite {
- /// Test-only: true while a mounted Ghostty surface still has an output consumer.
- func debugHasTerminalOutputSinkForTesting(surfaceID: String) -> Bool {
- hasTerminalOutputSink(surfaceID: surfaceID)
- }
-}
-#endifAs per path instructions, "**/Sources/**/*.swift: ... flag added test-only or debug-only seams ... members named like ...ForTesting."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if DEBUG | |
| extension MobileShellComposite { | |
| /// Test-only: true while a mounted Ghostty surface still has an output consumer. | |
| func debugHasTerminalOutputSinkForTesting(surfaceID: String) -> Bool { | |
| hasTerminalOutputSink(surfaceID: surfaceID) | |
| } | |
| } | |
| #endif |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+Testing.swift
around lines 1 - 8, The MobileShellComposite+Testing.swift file contains a
test-only seam with the debugHasTerminalOutputSinkForTesting method wrapped in
an `#if` DEBUG conditional, which violates the production-source seam policy for
Sources/ code. Remove the entire `#if` DEBUG block including the extension and the
debugHasTerminalOutputSinkForTesting wrapper method. Instead, rely on `@testable`
import in tests to directly access the internal hasTerminalOutputSink method,
eliminating the need for a test-only DEBUG seam in production source files.
Sources: Coding guidelines, Path instructions
| let collector = OutputCollector() | ||
| collector.mount(store: store, surfaceID: "live-terminal") | ||
| let initialReplayDelivered = try await pollUntil { | ||
| collector.lines.contains { $0.contains("PIXELCACHE") } | ||
| } | ||
| #expect(initialReplayDelivered) | ||
| #expect(store.debugHasTerminalOutputSinkForTesting(surfaceID: "live-terminal")) | ||
| let initialFrameBytes = try #require(collector.lines.last { $0.contains("PIXELCACHE") }) | ||
| let initialLineCount = collector.lines.count | ||
|
|
||
| store.debugRecoverMobileConnectionForTesting() | ||
|
|
||
| #expect(store.connectionState == .connected) | ||
| #expect(store.debugHasTerminalOutputSinkForTesting(surfaceID: "live-terminal")) | ||
| let replayRequestedAgain = try await pollUntil { | ||
| await router.count(of: "mobile.terminal.replay") >= 2 | ||
| } | ||
| #expect( | ||
| replayRequestedAgain, | ||
| "foreground recovery should repaint from the Mac without unregistering the local Ghostty sink" | ||
| ) | ||
| #expect(store.debugHasTerminalOutputSinkForTesting(surfaceID: "live-terminal")) | ||
|
|
||
| let cachedFrameStillPresent = try await pollUntil { | ||
| collector.lines.count > initialLineCount | ||
| && collector.lines.last { $0.contains("PIXELCACHE") } != nil | ||
| } | ||
| #expect( | ||
| cachedFrameStillPresent, | ||
| "the mounted sink must keep the last known render-grid frame available throughout reconnect" | ||
| ) | ||
| let recoveredFrameBytes = try #require(collector.lines.last { $0.contains("PIXELCACHE") }) | ||
| #expect( | ||
| recoveredFrameBytes == initialFrameBytes, | ||
| "the reconnect replay should be byte-for-byte identical for an unchanged render-grid frame" | ||
| ) | ||
|
|
||
| let event = try renderGridEventFrame(surfaceID: "live-terminal", seq: 2, text: "LIVEAFTER") | ||
| let transport = try #require(box.get()) | ||
| await transport.deliver(event) | ||
| let liveEventDelivered = try await pollUntil { | ||
| collector.lines.contains { $0.contains("LIVEAFTER") } | ||
| } | ||
| #expect(liveEventDelivered) | ||
| #expect(store.debugHasTerminalOutputSinkForTesting(surfaceID: "live-terminal")) | ||
| collector.unmount() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guarantee collector cleanup on early test failure paths.
Lines 202 and 233 can throw via #require; if either fails, Line 240 is skipped and the mounted collector task remains alive, which can leak state into subsequent tests. Add a defer right after mount.
Suggested fix
let collector = OutputCollector()
collector.mount(store: store, surfaceID: "live-terminal")
+ defer { collector.unmount() }
let initialReplayDelivered = try await pollUntil {
collector.lines.contains { $0.contains("PIXELCACHE") }
}
@@
- `#expect`(store.debugHasTerminalOutputSinkForTesting(surfaceID: "live-terminal"))
- collector.unmount()
+ `#expect`(store.debugHasTerminalOutputSinkForTesting(surfaceID: "live-terminal"))As per coding guidelines, “Tests must not depend on order of shared static / global / ... state that is not reset per test; use per-test isolated state with setUp + tearDown reset.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swift`
around lines 195 - 241, The test mounts a collector via the mount method but the
unmount cleanup call at the end can be skipped if an early assertion or
requirement fails (such as the `#require` calls), causing the mounted collector
task to persist and leak state into subsequent tests. Add a defer block
immediately after the collector.mount call to guarantee that collector.unmount
is always executed regardless of how the test exits, ensuring proper cleanup
even when early assertions fail.
Source: Coding guidelines
# Conflicts: # Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift # Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositePreviewTests.swift # Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift # Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileConnectionRecoveryBanner.swift # ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cfd0477. Configure here.
| && hasCachedRemoteWorkspaceSnapshot | ||
| && !connectionRequiresReauth | ||
| && (isRecoveringConnection || isReconnectingStoredMac) | ||
| && workspaces.contains { !$0.terminals.isEmpty } |
There was a problem hiding this comment.
Pairing errors still preserve shell
Medium Severity
shouldPreserveWorkspaceShellDuringReconnect stays true during stored-Mac reconnect when connectionError is set, because it only excludes connectionRequiresReauth. Root routing prefers WorkspaceShellView over restoring and onboarding, so explicit pairing failures can be hidden behind a cached terminal and reconnecting banner.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit cfd0477. Configure here.
| if group.wait(timeout: .now() + 0.6) == .timedOut { | ||
| return "===== visible terminal: (snapshot skipped — render busy) =====" | ||
| } |
There was a problem hiding this comment.
DispatchGroup.wait blocks the main thread up to 600 ms
visibleTerminalSnapshot() is a public synchronous function with no #if DEBUG guard. Calling group.wait(timeout: .now() + 0.6) on whatever thread the caller runs on — including the main thread — blocks it for up to 600 ms while waiting for per-executor render work to complete. Both current call sites are #if DEBUG-only, but the symbol is public and compiled into production. The fix is to make the function async and replace the DispatchGroup with per-executor withCheckedContinuation completions awaited via withTaskGroup, matching the existing processOutputAndWait pattern.
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalReplay.swift:
- Around line 20-49: The issue is that the code joins an in-flight replay task
without verifying it was created for the current remoteClient, causing stale
tasks to be reused when the connection changes. Modify the
terminalReplayRetryTasksBySurfaceID storage to track both the replay task and
the client it was created for (consider creating a structure to hold both
values). In the early return check for existingTask, compare the stored client
against the current remoteClient before joining; only join if they match,
otherwise proceed to create a new task for the current connection. Update the
cleanup section to clear the stored client alongside the task.
- Line 109: In the mobileShellReplayLog.error call within the CMUX_REPLAY error
logging statement, the interpolated error description is currently marked with
privacy: .public, which risks exposing sensitive information like tokens or
private endpoints. Change the privacy parameter for the String(describing:
error) interpolation from .public to .private, while keeping the surfaceID
privacy setting as .public, to ensure error details are properly redacted in
production logs.
- Around line 8-11: Add the nonisolated keyword to the mobileShellReplayLog
Logger declaration to mark it as nonisolated private let and avoid unnecessary
MainActor coupling in Swift 6. For the stale-client replay join bug in the
replay task logic around lines 20-25, modify the task dedup key to include the
client identity (such as remoteClient's reference or identity) so that when
remoteClient changes, a new caller receives a fresh replay task instead of
joining a stale task created with the old client. Finally, locate the error
logging statement around line 109 and change the privacy level from .public to
.private to prevent sensitive error details from being exposed in logs.
In
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellTerminalReplayTests.swift`:
- Around line 92-93: Remove the fixed Task.sleep call on line 92 and the similar
one on lines 141-143 from the test methods. Instead of sleeping for a fixed
duration before asserting, use predicate-based waits with existing signals such
as pollUntil, router counter checks, or collector output to wait for the actual
condition to be true. Replace the sleep-then-assert pattern with a mechanism
that polls the router counter (or other available signal) until it reaches the
expected value, ensuring the test waits for the actual readiness condition
rather than wall-clock time.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift`:
- Around line 189-229: The if-else branch chain in CMUXMobileRootView is
duplicating the routing logic that already exists in
MobileRootAuthGate.rootContentDestination(...), creating a risk of divergence in
authentication and connection state handling. Replace this entire branch chain
with a call to the authoritative MobileRootAuthGate.rootContentDestination(...)
method to establish a single source of truth for root content routing decisions,
ensuring consistent behavior across all reconnection, authentication, and
onboarding edge cases.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileConnectionRecoveryBanner.swift`:
- Around line 38-45: The L10n.string calls in
MobileConnectionRecoveryBanner.swift reference localization keys
(mobile.recovery.lost, mobile.recovery.lostDescription,
mobile.recovery.reconnecting, mobile.recovery.switchAccount,
mobile.recovery.retry) that are not registered in the string catalog. Add these
missing keys to Resources/Localizable.xcstrings with proper translations for all
supported locales (English and Japanese). Use the defaultValue strings from each
L10n.string call as the English translation, and provide corresponding Japanese
translations for each key to satisfy the localization requirement.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swift`:
- Around line 181-187: Add the missing localization keys to the
Resources/Localizable.xcstrings file. Create entries for both
mobile.loading.timeout.title and mobile.loading.timeout.message that are
currently referenced in the WorkspaceListView but not defined in the string
catalog. For each key, provide translations in all supported locales including
English (using the defaultValue strings already present in the code) and
Japanese.
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceRegistry.swift`:
- Around line 108-116: The current implementation uses DispatchQueue.asyncAfter
with fire-and-forget Task blocks which creates a race condition where both the
executor callback and the timeout task can independently call
continuationBox.resume(), causing a crash. Replace this pattern with a
cancellation-aware timeout mechanism using withTaskGroup or Task.withDeadline
that ties the timeout to the request lifecycle and ensures only one completion
path can fire. If a defensive timeout is genuinely necessary, restructure the
executor callback to guarantee a single completion path (either from the
executor itself or the timeout, but not both) and document why the executor
might not complete on its own.
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 2210-2216: The sleep-based timeout implemented with
ContinuousClock().sleep(for:) in the timeoutTask violates production runtime
code standards for shipped apps. Replace the timing-based wait mechanism that
uses duration calculated from Self.outputApplyTimeoutSeconds with state-driven
recovery signaling by implementing either a real timer abstraction, a
cancellation-aware scheduler, or explicit state transitions. This ensures that
the caller and state machine remain aware of when recovery completes and
prevents the sleep from blocking or delaying output delivery.
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView`+ReplayRecovery.swift:
- Around line 37-44: The recoveryReplayTask is implementing a 2-second timeout
using DispatchQueue.global(qos: .userInitiated).asyncAfter with a checked
continuation, which violates the policy against delayed-dispatch patterns in
shipped app code. Replace this approach with a cancellation-aware timer
abstraction, async sequence, or state-driven event. Remove the
DispatchQueue.asyncAfter call and the continuation.resume() inside the async
block, then refactor the timeout mechanism to use an appropriate
cancellation-aware timer implementation that respects Task cancellation
semantics.
In
`@Packages/iOS/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/TerminalSurfaceOutputWaitStateTests.swift`:
- Around line 34-35: The assertion on `cancelAll().map(\.id)` compares the
result as an ordered array, which is nondeterministic when cancelAll is backed
by unordered storage like a Set or Dictionary. Change the assertion to compare
the results as sets instead of relying on order by converting both sides to sets
(or sorting both sides before comparison) to ensure the test is deterministic
and does not fail randomly in CI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: caadb4ab-b3c9-4b7f-8b1a-7e2389c98927
📒 Files selected for processing (46)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ReconnectPresentation.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalReplay.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridReconnectTestSupport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellTerminalReplayTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalOutputDeliveryQueueTests.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileTerminalOutputSinking.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileConnectionRecoveryBanner.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileConnectionRecoveryBannerPresentation.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileConnectionRecoveryOverlay.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileRootAuthGate+ShellSync.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/CopyableTextContinuationBox.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/DisplayLinkProxy.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceBridgeRetain.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceRegistry.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LocalScrollbackScroll.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+OutputWaits.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+RenderWorkItems.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+ReplayRecovery.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+VisibleSnapshot.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceWorkExecutor.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceWorkHandle.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/VisibleTerminalSnapshotResultBox.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceOutputWaitState.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfacePresentation.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceRecoveryDecision.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceRenderCompletionDecision.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceRenderPhase.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceRenderRequestDecision.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceReplayAttemptDecision.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceReplayCompletionDecision.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceReplayRecovery.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceSessionState.swiftPackages/iOS/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/TerminalSurfaceOutputWaitStateTests.swiftPackages/iOS/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/TerminalSurfaceSessionStateTests.swiftPackages/iOS/CmuxMobileWorkspace/Sources/CmuxMobileWorkspace/MobileRootAuthGate.swiftPackages/iOS/CmuxMobileWorkspace/Sources/CmuxMobileWorkspace/MobileRootContentDestination.swiftPackages/iOS/CmuxMobileWorkspace/Tests/CmuxMobileWorkspaceTests/MobileRootAuthGateTests.swift
💤 Files with no reviewable changes (1)
- Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalReplay.swift:
- Around line 20-49: The issue is that the code joins an in-flight replay task
without verifying it was created for the current remoteClient, causing stale
tasks to be reused when the connection changes. Modify the
terminalReplayRetryTasksBySurfaceID storage to track both the replay task and
the client it was created for (consider creating a structure to hold both
values). In the early return check for existingTask, compare the stored client
against the current remoteClient before joining; only join if they match,
otherwise proceed to create a new task for the current connection. Update the
cleanup section to clear the stored client alongside the task.
- Line 109: In the mobileShellReplayLog.error call within the CMUX_REPLAY error
logging statement, the interpolated error description is currently marked with
privacy: .public, which risks exposing sensitive information like tokens or
private endpoints. Change the privacy parameter for the String(describing:
error) interpolation from .public to .private, while keeping the surfaceID
privacy setting as .public, to ensure error details are properly redacted in
production logs.
- Around line 8-11: Add the nonisolated keyword to the mobileShellReplayLog
Logger declaration to mark it as nonisolated private let and avoid unnecessary
MainActor coupling in Swift 6. For the stale-client replay join bug in the
replay task logic around lines 20-25, modify the task dedup key to include the
client identity (such as remoteClient's reference or identity) so that when
remoteClient changes, a new caller receives a fresh replay task instead of
joining a stale task created with the old client. Finally, locate the error
logging statement around line 109 and change the privacy level from .public to
.private to prevent sensitive error details from being exposed in logs.
In
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellTerminalReplayTests.swift`:
- Around line 92-93: Remove the fixed Task.sleep call on line 92 and the similar
one on lines 141-143 from the test methods. Instead of sleeping for a fixed
duration before asserting, use predicate-based waits with existing signals such
as pollUntil, router counter checks, or collector output to wait for the actual
condition to be true. Replace the sleep-then-assert pattern with a mechanism
that polls the router counter (or other available signal) until it reaches the
expected value, ensuring the test waits for the actual readiness condition
rather than wall-clock time.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift`:
- Around line 189-229: The if-else branch chain in CMUXMobileRootView is
duplicating the routing logic that already exists in
MobileRootAuthGate.rootContentDestination(...), creating a risk of divergence in
authentication and connection state handling. Replace this entire branch chain
with a call to the authoritative MobileRootAuthGate.rootContentDestination(...)
method to establish a single source of truth for root content routing decisions,
ensuring consistent behavior across all reconnection, authentication, and
onboarding edge cases.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileConnectionRecoveryBanner.swift`:
- Around line 38-45: The L10n.string calls in
MobileConnectionRecoveryBanner.swift reference localization keys
(mobile.recovery.lost, mobile.recovery.lostDescription,
mobile.recovery.reconnecting, mobile.recovery.switchAccount,
mobile.recovery.retry) that are not registered in the string catalog. Add these
missing keys to Resources/Localizable.xcstrings with proper translations for all
supported locales (English and Japanese). Use the defaultValue strings from each
L10n.string call as the English translation, and provide corresponding Japanese
translations for each key to satisfy the localization requirement.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swift`:
- Around line 181-187: Add the missing localization keys to the
Resources/Localizable.xcstrings file. Create entries for both
mobile.loading.timeout.title and mobile.loading.timeout.message that are
currently referenced in the WorkspaceListView but not defined in the string
catalog. For each key, provide translations in all supported locales including
English (using the defaultValue strings already present in the code) and
Japanese.
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceRegistry.swift`:
- Around line 108-116: The current implementation uses DispatchQueue.asyncAfter
with fire-and-forget Task blocks which creates a race condition where both the
executor callback and the timeout task can independently call
continuationBox.resume(), causing a crash. Replace this pattern with a
cancellation-aware timeout mechanism using withTaskGroup or Task.withDeadline
that ties the timeout to the request lifecycle and ensures only one completion
path can fire. If a defensive timeout is genuinely necessary, restructure the
executor callback to guarantee a single completion path (either from the
executor itself or the timeout, but not both) and document why the executor
might not complete on its own.
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 2210-2216: The sleep-based timeout implemented with
ContinuousClock().sleep(for:) in the timeoutTask violates production runtime
code standards for shipped apps. Replace the timing-based wait mechanism that
uses duration calculated from Self.outputApplyTimeoutSeconds with state-driven
recovery signaling by implementing either a real timer abstraction, a
cancellation-aware scheduler, or explicit state transitions. This ensures that
the caller and state machine remain aware of when recovery completes and
prevents the sleep from blocking or delaying output delivery.
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView`+ReplayRecovery.swift:
- Around line 37-44: The recoveryReplayTask is implementing a 2-second timeout
using DispatchQueue.global(qos: .userInitiated).asyncAfter with a checked
continuation, which violates the policy against delayed-dispatch patterns in
shipped app code. Replace this approach with a cancellation-aware timer
abstraction, async sequence, or state-driven event. Remove the
DispatchQueue.asyncAfter call and the continuation.resume() inside the async
block, then refactor the timeout mechanism to use an appropriate
cancellation-aware timer implementation that respects Task cancellation
semantics.
In
`@Packages/iOS/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/TerminalSurfaceOutputWaitStateTests.swift`:
- Around line 34-35: The assertion on `cancelAll().map(\.id)` compares the
result as an ordered array, which is nondeterministic when cancelAll is backed
by unordered storage like a Set or Dictionary. Change the assertion to compare
the results as sets instead of relying on order by converting both sides to sets
(or sorting both sides before comparison) to ensure the test is deterministic
and does not fail randomly in CI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: caadb4ab-b3c9-4b7f-8b1a-7e2389c98927
📒 Files selected for processing (46)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ReconnectPresentation.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalReplay.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridReconnectTestSupport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellTerminalReplayTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalOutputDeliveryQueueTests.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileTerminalOutputSinking.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileConnectionRecoveryBanner.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileConnectionRecoveryBannerPresentation.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileConnectionRecoveryOverlay.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileRootAuthGate+ShellSync.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/CopyableTextContinuationBox.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/DisplayLinkProxy.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceBridgeRetain.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceRegistry.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LocalScrollbackScroll.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+OutputWaits.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+RenderWorkItems.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+ReplayRecovery.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+VisibleSnapshot.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceWorkExecutor.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceWorkHandle.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/VisibleTerminalSnapshotResultBox.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceOutputWaitState.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfacePresentation.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceRecoveryDecision.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceRenderCompletionDecision.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceRenderPhase.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceRenderRequestDecision.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceReplayAttemptDecision.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceReplayCompletionDecision.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceReplayRecovery.swiftPackages/iOS/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/TerminalSurfaceSessionState.swiftPackages/iOS/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/TerminalSurfaceOutputWaitStateTests.swiftPackages/iOS/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/TerminalSurfaceSessionStateTests.swiftPackages/iOS/CmuxMobileWorkspace/Sources/CmuxMobileWorkspace/MobileRootAuthGate.swiftPackages/iOS/CmuxMobileWorkspace/Sources/CmuxMobileWorkspace/MobileRootContentDestination.swiftPackages/iOS/CmuxMobileWorkspace/Tests/CmuxMobileWorkspaceTests/MobileRootAuthGateTests.swift
💤 Files with no reviewable changes (1)
- Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift
🛑 Comments failed to post (11)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalReplay.swift (3)
8-11: 📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalReplay.swiftRepository: manaflow-ai/cmux
Length of output: 6623
Address replay task join stale-client issue and logger isolation.
This file has three issues:
Logger must be
nonisolated(line 8): File-scopedLoggershould benonisolated private letto avoid unnecessary MainActor coupling in Swift 6.Stale-client replay join bug (lines 20–25): When
remoteClientchanges while a replay task is in-flight, a new caller joins the stale task (created with the old client). The task's request uses the captured old client, the stale-client guard at line 67 fails, and both callers incorrectly receivefalse. The new caller should receive a fresh replay on the current client. Fix: include client identity in the task dedup key, or check client freshness before joining.Error logging privacy leak (line 109): Error details are logged as
.public, exposing potentially sensitive information (network failures, auth errors, paths). Use.privateinstead.Logger fix
-private let mobileShellReplayLog = Logger( +nonisolated private let mobileShellReplayLog = Logger( subsystem: Bundle.main.bundleIdentifier ?? "dev.cmux.ios", category: "mobile-shell" )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.nonisolated private let mobileShellReplayLog = Logger( subsystem: Bundle.main.bundleIdentifier ?? "dev.cmux.ios", category: "mobile-shell" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalReplay.swift around lines 8 - 11, Add the nonisolated keyword to the mobileShellReplayLog Logger declaration to mark it as nonisolated private let and avoid unnecessary MainActor coupling in Swift 6. For the stale-client replay join bug in the replay task logic around lines 20-25, modify the task dedup key to include the client identity (such as remoteClient's reference or identity) so that when remoteClient changes, a new caller receives a fresh replay task instead of joining a stale task created with the old client. Finally, locate the error logging statement around line 109 and change the privacy level from .public to .private to prevent sensitive error details from being exposed in logs.Source: Coding guidelines
20-49: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t join replay tasks created for a stale remote client.
Line 20 joins any in-flight replay for the surface before checking whether it was created for the current
remoteClient. If the connection changes while an old replay is pending, the new replay request joins the old task; that task then fails theremoteClient === clientguard on Line 67 and no replay is started for the new connection. Track the client/generation in the replay slot and only join when it matches the current connection.As per path instructions, terminal replay/recovery/liveness must use one authoritative structured source of truth and fail closed when the reliable signal is stale.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalReplay.swift around lines 20 - 49, The issue is that the code joins an in-flight replay task without verifying it was created for the current remoteClient, causing stale tasks to be reused when the connection changes. Modify the terminalReplayRetryTasksBySurfaceID storage to track both the replay task and the client it was created for (consider creating a structure to hold both values). In the early return check for existingTask, compare the stored client against the current remoteClient before joining; only join if they match, otherwise proceed to create a new task for the current connection. Update the cleanup section to clear the stored client alongside the task.Source: Path instructions
109-109: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact replay errors in production logs.
String(describing: error)can include transport/auth details; logging it as.publicrisks exposing tokens or private endpoints. Keep the failure visible but mark the interpolated error private.Proposed fix
- mobileShellReplayLog.error("CMUX_REPLAY failed surface=\(surfaceID, privacy: .public) error=\(String(describing: error), privacy: .public)") + mobileShellReplayLog.error("CMUX_REPLAY failed surface=\(surfaceID, privacy: .public) error=\(String(describing: error), privacy: .private)")As per coding guidelines, logs must not expose secrets, tokens, customer content, or personal data without explicit private redaction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalReplay.swift at line 109, In the mobileShellReplayLog.error call within the CMUX_REPLAY error logging statement, the interpolated error description is currently marked with privacy: .public, which risks exposing sensitive information like tokens or private endpoints. Change the privacy parameter for the String(describing: error) interpolation from .public to .private, while keeping the surfaceID privacy setting as .public, to ensure error details are properly redacted in production logs.Source: Coding guidelines
Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellTerminalReplayTests.swift (1)
92-93: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove fixed
Task.sleepreadiness waits from these tests.Line 92 and Line 142 use wall-clock sleeps to gate assertions. This makes the tests timing-sensitive and flaky; switch to causality/predicate waits using existing signals (
pollUntil, router counters, collector output).As per coding guidelines, “Tests must not introduce fixed
sleep/Task.sleepcalls used to wait for async readiness before an assertion.”Suggested change
- try await Task.sleep(nanoseconds: 50_000_000) - `#expect`(await router.count(of: "mobile.terminal.replay") == 1) + let noDuplicateReplayRequest = try await pollUntil { + await router.count(of: "mobile.terminal.replay") == 1 + } + `#expect`(noDuplicateReplayRequest) @@ - await router.releaseHeldReplayRequest(number: 1) - try await Task.sleep(nanoseconds: 100_000_000) - `#expect`(store.terminalReplayRetryTaskIDsBySurfaceID["live-terminal"] == newerTaskID) + await router.releaseHeldReplayRequest(number: 1) + let orphanedCompletionDidNotClobberNewerTask = try await pollUntil { + collector.lines.contains { $0.contains("ORPHANED-REPLAY") } && + store.terminalReplayRetryTaskIDsBySurfaceID["live-terminal"] == newerTaskID + } + `#expect`(orphanedCompletionDidNotClobberNewerTask)Also applies to: 141-143
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellTerminalReplayTests.swift` around lines 92 - 93, Remove the fixed Task.sleep call on line 92 and the similar one on lines 141-143 from the test methods. Instead of sleeping for a fixed duration before asserting, use predicate-based waits with existing signals such as pollUntil, router counter checks, or collector output to wait for the actual condition to be true. Replace the sleep-then-assert pattern with a mechanism that polls the router counter (or other available signal) until it reaches the expected value, ensuring the test waits for the actual readiness condition rather than wall-clock time.Source: Coding guidelines
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift (1)
189-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use one authoritative root-routing source instead of duplicating branch policy in-view.
This branch chain re-implements correctness-critical destination logic that already exists in
MobileRootAuthGate.rootContentDestination(...). Keeping both paths live risks drift (wrong root destination in reconnect/auth edge states).Suggested refactor
`@ViewBuilder` private var rootContent: some View { - if shouldShowTerminalLayoutPreview { - terminalLayoutPreview - } else if shouldShowWorkspaceListLayoutPreview { - workspaceListLayoutPreview - } else if !isAuthenticated { - SignInView() - } else if store.shouldPreserveWorkspaceShellDuringReconnect { - WorkspaceShellView(store: store, signOut: signOut, showAddDevice: showAddDevice) - } else if store.connectionState != .connected && shouldShowRestoringStoredMac { - RestoringStoredMacWorkspaceShell(...) - } else if shouldShowOnboarding { - onboardingFlow - } else if store.connectionState != .connected && !store.hasKnownPairedMac { - DisconnectedWorkspaceShellView(...) - } else { - WorkspaceShellView(store: store, signOut: signOut, showAddDevice: showAddDevice) - } + switch MobileRootAuthGate.rootContentDestination( + showsTerminalLayoutPreview: shouldShowTerminalLayoutPreview, + showsWorkspaceListLayoutPreview: shouldShowWorkspaceListLayoutPreview, + showsRestoringSession: false, + authenticated: isAuthenticated, + preservesWorkspaceShellDuringReconnect: store.shouldPreserveWorkspaceShellDuringReconnect, + connectionState: store.connectionState, + showsRestoringStoredMac: shouldShowRestoringStoredMac, + hasKnownPairedMac: store.hasKnownPairedMac, + isReconnectingStoredMac: store.isReconnectingStoredMac, + showsOnboarding: shouldShowOnboarding + ) { + // map enum cases to views + } }As per path instructions, apply
reliability-single-source-of-truth.md: correctness-critical routing should use one authoritative structured source.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift` around lines 189 - 229, The if-else branch chain in CMUXMobileRootView is duplicating the routing logic that already exists in MobileRootAuthGate.rootContentDestination(...), creating a risk of divergence in authentication and connection state handling. Replace this entire branch chain with a call to the authoritative MobileRootAuthGate.rootContentDestination(...) method to establish a single source of truth for root content routing decisions, ensuring consistent behavior across all reconnection, authentication, and onboarding edge cases.Source: Path instructions
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileConnectionRecoveryBanner.swift (1)
38-45: 📐 Maintainability & Code Quality | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail XCSTRINGS="Resources/Localizable.xcstrings" keys=( "mobile.recovery.lost" "mobile.recovery.lostDescription" "mobile.recovery.reconnecting" "mobile.recovery.switchAccount" "mobile.recovery.retry" ) echo "Checking key presence..." for k in "${keys[@]}"; do jq -e --arg k "$k" '.strings[$k]' "$XCSTRINGS" >/dev/null \ && echo "OK: $k" \ || echo "MISSING: $k" done echo echo "Checking locale coverage (expect at least: en, ja)..." for k in "${keys[@]}"; do echo "== $k ==" jq -r --arg k "$k" '.strings[$k].localizations | keys[]?' "$XCSTRINGS" | sort doneRepository: manaflow-ai/cmux
Length of output: 519
Add missing localization keys to
Resources/Localizable.xcstringswith translations for all supported locales.The code references localization keys (
mobile.recovery.lost,mobile.recovery.lostDescription,mobile.recovery.reconnecting,mobile.recovery.switchAccount,mobile.recovery.retry) that do not exist in the string catalog. WhiledefaultValueprovides an English fallback at runtime, the keys must be registered inResources/Localizable.xcstringswith translated entries for every supported locale (English and Japanese) to satisfy the localization requirement.Also applies to: 51-54, 109, 189, 212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileConnectionRecoveryBanner.swift` around lines 38 - 45, The L10n.string calls in MobileConnectionRecoveryBanner.swift reference localization keys (mobile.recovery.lost, mobile.recovery.lostDescription, mobile.recovery.reconnecting, mobile.recovery.switchAccount, mobile.recovery.retry) that are not registered in the string catalog. Add these missing keys to Resources/Localizable.xcstrings with proper translations for all supported locales (English and Japanese). Use the defaultValue strings from each L10n.string call as the English translation, and provide corresponding Japanese translations for each key to satisfy the localization requirement.Sources: Coding guidelines, Path instructions
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swift (1)
181-187: 📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail XCSTRINGS="Resources/Localizable.xcstrings" keys=( "mobile.loading.timeout.title" "mobile.loading.timeout.message" ) for k in "${keys[@]}"; do echo "== $k ==" jq -e --arg k "$k" '.strings[$k]' "$XCSTRINGS" >/dev/null \ && echo "present" || echo "MISSING" echo "locales:" jq -r --arg k "$k" '.strings[$k].localizations | keys[]?' "$XCSTRINGS" | sort echo doneRepository: manaflow-ai/cmux
Length of output: 285
Add localization keys for timeout messages to the string catalog.
The code routes through
L10n.string()but the keysmobile.loading.timeout.titleandmobile.loading.timeout.messageare missing fromResources/Localizable.xcstrings. Add entries for both keys with translations in all supported locales (English and Japanese).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swift` around lines 181 - 187, Add the missing localization keys to the Resources/Localizable.xcstrings file. Create entries for both mobile.loading.timeout.title and mobile.loading.timeout.message that are currently referenced in the WorkspaceListView but not defined in the string catalog. For each key, provide translations in all supported locales including English (using the defaultValue strings already present in the code) and Japanese.Sources: Coding guidelines, Path instructions
Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceRegistry.swift (1)
108-116: 🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file and understand the context ast-grep outline Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceRegistry.swiftRepository: manaflow-ai/cmux
Length of output: 513
🏁 Script executed:
# Read the specific section with more context sed -n '95,130p' Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceRegistry.swiftRepository: manaflow-ai/cmux
Length of output: 1378
🏁 Script executed:
# Get the full function/method context sed -n '80,150p' Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceRegistry.swiftRepository: manaflow-ai/cmux
Length of output: 2345
Use a cancellation-aware timeout mechanism instead of
DispatchQueue.asyncAfterand fire-and-forgetTaskblocks.Lines 108-116 use
DispatchQueue.global(qos: .userInitiated).asyncAfterwith a 1-second timeout and two separate fire-and-forgetTaskblocks to resume the continuation. This pattern violates two Swift runtime guidelines:
- Timing-based repair path:
asyncAfterintroduces an unowned, untrackable delay in production code to paper over a potential executor hang.- Fire-and-forget Tasks with meaningful lifecycle: Both the executor callback (lines 108–110) and the timeout task (lines 113–115) can independently call
continuationBox.resume(), creating a race where both fires and crashes with "tried to resume an already-resumed continuation."Replace this with a cancellation-aware timeout that is tied to the request lifecycle. Use
withTaskGrouporTask.withDeadlineto ensure the timeout cancels the executor task once text is retrieved, or re-architect the executor callback to guarantee a single completion path without a separate timeout fallback. Document why the executor might not complete if a defensive timeout is genuinely unavoidable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceRegistry.swift` around lines 108 - 116, The current implementation uses DispatchQueue.asyncAfter with fire-and-forget Task blocks which creates a race condition where both the executor callback and the timeout task can independently call continuationBox.resume(), causing a crash. Replace this pattern with a cancellation-aware timeout mechanism using withTaskGroup or Task.withDeadline that ties the timeout to the request lifecycle and ensures only one completion path can fire. If a defensive timeout is genuinely necessary, restructure the executor callback to guarantee a single completion path (either from the executor itself or the timeout, but not both) and document why the executor might not complete on its own.Sources: Coding guidelines, Path instructions
Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift (1)
2210-2216: 🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
cmux iOS support status shipped product 2024 2025💡 Result:
As of June 2026, official iOS support for the cmux terminal multiplexer has been in active development and integration throughout 2025 and 2026, transitioning from third-party companion prototypes to a first-party Swift-based mobile terminal [1][2]. Key status updates include: First-Party iOS Integration (2025-2026): The official cmux project (manaflow-ai/cmux) has been actively developing a native Swift iOS/iPadOS terminal client [1][2]. This implementation uses a custom CMUXMobileCore for protocol handling, Network.framework for TCP transport, and Ghostty-based rendering [1][2]. Recent 2026 updates have focused on stabilizing mobile RPC, authentication flows, and onboarding guidance [1][3]. Transport and Connectivity: The official iOS client has historically relied on Tailscale for secure, remote connectivity between the iOS device and a Mac host [4][3]. As of mid-2026, there is an active transition to adopt the iroh protocol (QUIC-based) as the default transport for iOS-to-Mac communication, with Tailscale being relegated to an optional, opt-in fallback [5]. Third-Party Companions (2024-2025): Prior to and alongside official efforts, the ecosystem saw third-party companion apps emerge to bridge the gap [6][7]. Projects such as cmux-mobile and cmux-companion provided functionality like terminal monitoring, remote control, and workspace management, often requiring a separate bridge binary running on the host Mac to translate local Unix socket APIs into network-accessible streams [6][7]. In summary, while community-driven companion apps provided early mobile access during 2024 and 2025, the project transitioned to a native, first-party iOS terminal experience in 2026, integrating directly with the core application architecture [1][2].
Citations:
- 1: #4055
- 2: #5079
- 3: #5714
- 4: #5722
- 5: #5735
- 6: https://github.com/richardhowes/cmux-mobile
- 7: https://github.com/itsmaleen/cmux-companion
Replace sleep-based output timeout with state-driven recovery signaling.
iOS is a shipped product as of 2026. This path uses
ContinuousClock().sleep(for:)to implement output-apply timeout in production runtime code, which violates.github/review-bot-rules/swift-blocking-runtime.md. Timing-based waits (Task.sleep, delayed dispatch, polling-style waits) in shipped app/runtime code must be replaced with state-driven recovery signaling, real timer abstractions, cancellation-aware schedulers, or explicit state transitions. A simple sleep-based timeout leaves the caller and state machine unaware of when recovery completes and can block or delay output delivery.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift` around lines 2210 - 2216, The sleep-based timeout implemented with ContinuousClock().sleep(for:) in the timeoutTask violates production runtime code standards for shipped apps. Replace the timing-based wait mechanism that uses duration calculated from Self.outputApplyTimeoutSeconds with state-driven recovery signaling by implementing either a real timer abstraction, a cancellation-aware scheduler, or explicit state transitions. This ensures that the caller and state machine remain aware of when recovery completes and prevents the sleep from blocking or delaying output delivery.Sources: Coding guidelines, Path instructions
Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+ReplayRecovery.swift (1)
37-44: 🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+ReplayRecovery.swift | head -60Repository: manaflow-ai/cmux
Length of output: 3370
🏁 Script executed:
# Check if there are other recovery timeout patterns or Timer usage in the file rg -n "timeout|Timer|sleep|asyncAfter" Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+ReplayRecovery.swift -A 2 -B 2Repository: manaflow-ai/cmux
Length of output: 749
🏁 Script executed:
# Check the Recovery Timeout constant and understand the intended behavior rg -n "recoveryReplayApplyTimeoutSeconds" Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/ -B 3 -A 3Repository: manaflow-ai/cmux
Length of output: 2247
Remove
DispatchQueue.asyncAfterfrom recovery replay timeout; use a cancellation-aware timer abstraction.Line 39 uses
DispatchQueue.global(qos: .userInitiated).asyncAfterto implement a 2-second replay apply timeout. This violates the policy against delayed-dispatch delays in shipped app code. Even with structured state checks and explicit fail-over logic, async timing in recovery/repair paths must use a proper cancellation-aware timer abstraction, async sequence, or state-driven event rather thanDispatchQueue.asyncAfterorTask.sleep.Per
.github/review-bot-rules/swift-blocking-runtime.md, "FlagDispatchQueue.asyncAfter, timers, or polling loops in shipped app/runtime Swift code as failures by default, even for small delays" and "Retry backoff, keepalive loops, readiness waits, delayed dispatch, and polling require a real cancellation-aware scheduler, timer abstraction, async sequence, callback, notification, or state transition."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView`+ReplayRecovery.swift around lines 37 - 44, The recoveryReplayTask is implementing a 2-second timeout using DispatchQueue.global(qos: .userInitiated).asyncAfter with a checked continuation, which violates the policy against delayed-dispatch patterns in shipped app code. Replace this approach with a cancellation-aware timer abstraction, async sequence, or state-driven event. Remove the DispatchQueue.asyncAfter call and the continuation.resume() inside the async block, then refactor the timeout mechanism to use an appropriate cancellation-aware timer implementation that respects Task cancellation semantics.Sources: Coding guidelines, Path instructions
Packages/iOS/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/TerminalSurfaceOutputWaitStateTests.swift (1)
34-35: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid order-dependent assertion for cancellation results.
cancelAll().map(\.id)can be nondeterministic when backed by unordered storage, which risks flaky CI. Compare as a set (or sort before asserting).Suggested fix
- `#expect`(waits.cancelAll().map(\.id) == [first, second]) + `#expect`(Set(waits.cancelAll().map(\.id)) == Set([first, second]))As per coding guidelines, "Tests must not assert an ordered result of an unordered Set / Dictionary (or equivalent); sort or compare as sets instead."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/TerminalSurfaceOutputWaitStateTests.swift` around lines 34 - 35, The assertion on `cancelAll().map(\.id)` compares the result as an ordered array, which is nondeterministic when cancelAll is backed by unordered storage like a Set or Dictionary. Change the assertion to compare the results as sets instead of relying on order by converting both sides to sets (or sorting both sides before comparison) to ensure the test is deterministic and does not fail randomly in CI.Source: Coding guidelines
|
Found 1 test failure on Blacksmith runners: Failure
|



Summary
Validation
swift test --package-path Packages/iOS/CmuxMobileShell --filter MobileShellCompositePreviewTests./scripts/reload-cloud.sh --tag iosrcios/scripts/reload-cloud.sh --tag iosrc --device-id E4058DA9-F4C7-52DD-951D-0354061B8E89 --waitdev.cmux.ios.iosrcon iPhone with attach deeplink and verifiedmobile.host.statusactive_connection_count=1Dogfood
Use tag
iosrc. Background the iOS app while a terminal is visible, then return. The previous bad behavior was a blank/reset shell because the root view unmounted the workspace shell. The expected behavior is the last terminal frame remains visible with the reconnecting banner while transport recovers.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
High Risk
Large changes to iOS terminal rendering, output sequencing, and reconnect routing; regressions could show stale frames, miss live updates, or wedge recovery if replay/ack logic is wrong.
Overview
Reconnect UX: When a real remote workspace snapshot exists, the root view keeps
WorkspaceShellViewmounted instead of swapping to the restoring shell, and the recovery banner can show Reconnecting… over that cached UI (not only whileisRecoveringConnectionis true). The snapshot flag is set when workspaces have terminals, cleared on sign-out, and preservation is skipped when re-auth is required.Shell ↔ terminal sync: Terminal replay is centralized in
performTerminalReplay(deduped in-flight RPCs). Output delivery tracksendSeq, only advances delivered sequence after the surface acks (terminalOutputDidProcess), andterminalOutputDidDropForRetryresets queues so dropped chunks do not ack bytes that never rendered. Connect, secondary promote, and resync paths explicitly replay already-mounted sinks. Per-surface replay exhaustion surfaces the existing Retry banner without clearing failures on sibling terminals.Ghostty on device: Rendering moves to per-generation serial executors with coalesced
render_now, timeouts, and abandon-and-rebuild on wedged output/render; the last live frame or text snapshot stays visible while bounded replay runs via new delegate hooks fromGhosttySurfaceRepresentable.processOutputAndWaitnow returns whether output applied; failed applies trigger drop-for-retry and replay.Tests / UI test harness: New coverage for preserved-shell reconnect replay, foreground recovery with mounted render grids, replay joining/orphans, output queue drop/seq behavior, recovery banner presentation, paired-Mac hint when store is nil, and launch-argument fallbacks for UI test config.
Reviewed by Cursor Bugbot for commit 42dcaff. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Keep the iOS terminal visible during reconnects by preserving a cached workspace snapshot and showing a reconnecting banner over it. Adds Ghostty recovery with authoritative replay so the last frame stays on-screen until replay or live updates arrive, plus a UI test that verifies frame preservation across background/foreground.
New Features
MobileShellComposite.performTerminalReplay(surfaceID:)issues authoritative replays after local surface rebuilds; output delivery tracksendSeqand requests replay when chunks drop.shouldPreserveWorkspaceShellDuringReconnectgates shell preservation on real cached snapshots; cache clears on sign‑out/forget‑Mac and is disabled for reauth/pairing.MobileConnectionRecoveryBanneruses a simple presentation model and shows “Reconnecting…” when a preserved snapshot exists or recovery is active.Refactors
MobileRootAuthGate.RootContentDestination;CMUXMobileRootViewmountsWorkspaceShellViewwhen preserving.GhosttySurfaceRepresentableapplies output with async backpressure and triggers replay on dropped chunks; minor shell utilities split out (e.g.,placeholderHostName).Written for commit 42dcaff. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes